Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Replace pkg_resources.parse_version with packaging.version.parse #693

Merged
merged 7 commits into from
Mar 10, 2023

Conversation

ijnek
Copy link
Member

@ijnek ijnek commented Feb 2, 2023

Resolves: #678.

Switch to packaging since use of pkg_resources is discouraged. Related: pypa/setuptools#2497

Also:

  • Uses regex to do version matching, because packaging's version parsing is a lot more strict about version numbers.
  • Also, removes a check for whether a mac is Mountain Lion or greater and just changes it to a check for whether it is a mac.

Signed-off-by: ijnek <kenjibrameld@gmail.com>
Signed-off-by: ijnek <kenjibrameld@gmail.com>
Signed-off-by: ijnek <kenjibrameld@gmail.com>
Signed-off-by: ijnek <kenjibrameld@gmail.com>
@ijnek ijnek changed the title Replace parse_version with packaging.version.parse Replace pkg_resources.parse_version with packaging.version.parse Feb 2, 2023
@ijnek ijnek marked this pull request as ready for review February 2, 2023 04:07
bloom/logging.py Outdated Show resolved Hide resolved
Co-authored-by: Jochen Sprickerhof <github@jochen.sprickerhof.de>
Copy link
Contributor

@nuclearsandwich nuclearsandwich left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks @ijnek for this PR! and @jspricke for an earlier review!

While I'm not thrilled about the introduction of an additional dependency, it is one that is part of the extended suite of python packaging related dependencies and is widely packaged and available so it's not overly concerning.

This switch also resolves CI which is currently failing due to an apparent behavior change in how the deprecated pkg_resources function handles empty strings.

I have a couple of non-blocking suggestions but found one regex that needs feedback as I think it's incorrect as-is.

I also raised a question or two of general maintenance surfaced by looking at some changed areas which do not block merging this PR.

Once the regex is looked at again this should be quite close to ready.

@@ -39,7 +39,7 @@
import tarfile
import tempfile

from pkg_resources import parse_version
from packaging.version import parse
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Being a bit of a python outsider, I've always disliked that a function's utility is sometimes obfuscated when the functions are imported outside their module.
Does using the alias-on-import feature make sense so that parse doesn't lose its context.
If using the alias-on-import feature is not seen as idiomatic then I could live with it as is, but I think it improves local legibility.

Suggested change
from packaging.version import parse
from packaging.version import parse as parse_version

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Another alternative that is used in the other files is to import the entire version module which would afford the same context as the function would be invoked as version.parse().

Suggested change
from packaging.version import parse
from packaging import version

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I would say the function name should just be something more specific than parse, but I think your first suggestion is just fine for now.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It looks like I've avoided using from packaging import version, because the parameter for the version_check method was also version and there was a conflict.

But I can go with parse_version.

bloom/commands/git/import_upstream.py Outdated Show resolved Hide resolved
bloom/generators/release.py Outdated Show resolved Hide resolved
bloom/git.py Outdated
@@ -686,7 +687,9 @@ def get_last_tag_by_version(directory=None):
versions = []
for line in output.splitlines():
tags.append(line.strip())
versions.append(parse_version(line.strip()))
ver = re.match("[0-9]+.[0-9]+.[0-9]+", line)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
ver = re.match("[0-9]+.[0-9]+.[0-9]+", line)
ver = re.match(r"[0-9]+\.[0-9]+\.[0-9]+", line)

Since dots are regex metacharacters they need to be escaped for an exact match. Or is the intent that any character could separate these digits?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is likely what was intended.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Resolved in cd20317

bloom/logging.py Show resolved Hide resolved
Signed-off-by: ijnek <kenjibrameld@gmail.com>
…n" is used as variables everywhere

Signed-off-by: ijnek <kenjibrameld@gmail.com>
@ijnek
Copy link
Member Author

ijnek commented Mar 6, 2023

Across all files, I've gone with from packaging.version import parse as parse_version, because the parse isn't descriptive enough, and version.parse causes a lot of conflicts with local variables and parameters named version.

Copy link
Contributor

@nuclearsandwich nuclearsandwich left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for iterating @ijnek and for this excellent improvement. Among other things I believe this addresses the issue that is affecting CI for all Bloom PRs right now.

@nuclearsandwich nuclearsandwich merged commit b34dad4 into ros-infrastructure:master Mar 10, 2023
@ijnek ijnek deleted the ijnek-#678 branch March 10, 2023 01:46
nuclearsandwich added a commit that referenced this pull request Mar 10, 2023
This was introduced in #693 and I realized that it needed the stdeb
dependency while I was on a walk but forgot to add that before
re-reviewing and merging that PR.

This package is available for Python 2 in
[Buster](https://packages.debian.org/buster/python3-packaging) and
[Bionic](https://packages.ubuntu.com/bionic/python-packaging).

As well as for Python 3 in
[Buster](https://packages.debian.org/buster/python3-packaging) and
[Bionic](https://packages.ubuntu.com/bionic/python3-packaging) and later
distributions.
nuclearsandwich added a commit that referenced this pull request Apr 12, 2023
This was introduced in #693 and I realized that it needed the stdeb
dependency while I was on a walk but forgot to add that before
re-reviewing and merging that PR.

This package is available for Python 2 in
[Buster](https://packages.debian.org/buster/python3-packaging) and
[Bionic](https://packages.ubuntu.com/bionic/python-packaging).

As well as for Python 3 in
[Buster](https://packages.debian.org/buster/python3-packaging) and
[Bionic](https://packages.ubuntu.com/bionic/python3-packaging) and later
distributions.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

PkgResourcesDeprecationWarning: is an invalid version and will not be supported in a future release
4 participants